TransactionView: ReceiveAndBuffer #3820

apfitzge · 2024-11-27T16:50:50Z

Problem

Need to ingest new tx type into banking stage

Summary of Changes

Implement a new TransactionViewReceiveAndBuffer to allow banking stage to ingest TransactionView-based transactions
Add argument to BankingStage::new to switch between transaction representations
Hook up CLI for validator and relevant benchmarks
Run all BankingStage and SchedulerController tests using both options

Fixes #

apfitzge · 2024-12-02T17:13:29Z

core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs

+    type Transaction = RuntimeTransaction<ResolvedTransactionView<Bytes>>;
+    type Container = TransactionViewStateContainer;
+
+    fn receive_and_buffer_packets(


NOTE: this does not run the bank.check_transactions because I prioritized safety around not leaking Bytes in the container, which is much trickier if we do batching.
We should probably follow this up with a call to clean immediately after, so that we check age on transactions we actually inserted, which is likely better anyway. However, I'd like to profile with and without that change first.

apfitzge · 2024-12-03T16:05:45Z

core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs

+        if container.is_empty()
+            && matches!(
+                decision,
+                BufferedPacketsDecision::Forward | BufferedPacketsDecision::ForwardAndHold


removed Hold branch here. Sometimes after a leader slot, we do not have a bank ready in Consume for our next leader slot. We should still avoid the recv_timeout in that case. Also added a check that container is not empty - if container has items, then we should try to process them instead of sleeping for packets

pgarg66

Looks good from SVM point of view.

bw-solana

No major qualms with what you have. Left a few comments to consider

core/src/banking_stage.rs

validator/src/main.rs

bw-solana · 2024-12-09T16:17:54Z

core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs

+        };
+
+        // Receive packet batches.
+        const TIMEOUT: Duration = Duration::from_millis(10);


I'm thinking this will need to be profiled and tweaked. E.g. pull for a certain amount of time or up to some packet limit to make sure we don't spend all our time receiving and cleaning (and thus less time on actually scheduling to feed the workers)

Yeah. Want to make these more flexible instead of just constants.

After this PR lands, I have #4064 to hoist all the prio-graph constants into a config.
I am hoping to do something similar for the controller's timeouts.

bw-solana · 2024-12-09T16:19:32Z

core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs

+        timing_metrics: &mut SchedulerTimingMetrics,
+        count_metrics: &mut SchedulerCountMetrics,
+        decision: &BufferedPacketsDecision,
+    ) -> bool {


would be nice to add a comment or something indicating what this return bool signifies

there's a comment on the trait fn. I guess my editor will show me that comment if I hover this function.
Do you prefer I just copy the description to all (only 2) implementations of this trait?

If it's in the trait, I think what you have is fine

jstarry

Still reviewing but posting some feedback along the way

jstarry · 2024-12-04T03:28:54Z

core/src/banking_stage/transaction_scheduler/transaction_state.rs

+        let should_forward = !packet
+            .as_ref()
+            .map(|packet| {
+                packet.original_packet().meta().forwarded()
+                    && packet.original_packet().meta().is_from_staked_node()
+            })
+            .unwrap_or_default();


Suggested change

let should_forward = !packet

.as_ref()

.map(|packet| {

packet.original_packet().meta().forwarded()

&& packet.original_packet().meta().is_from_staked_node()

})

.unwrap_or_default();

let should_forward = packet

.as_ref()

.map(|packet| {

!packet.original_packet().meta().forwarded()

&& packet.original_packet().meta().is_from_staked_node()

})

.unwrap_or_default();

I think we should add some unit tests for this

65ca564
+
459b9e9

jstarry · 2024-12-04T03:31:41Z

core/src/banking_stage/transaction_scheduler/transaction_state_container.rs

        &mut self,
        transaction_ttl: SanitizedTransactionTTL<Tx>,
-        packet: Arc<ImmutableDeserializedPacket>,
+        packet: Option<Arc<ImmutableDeserializedPacket>>,


nit: we never pass None here, how about Option wrapping before passing to TransactionState::new?

yeah - was originally calling this from the other variant of ReceiveAndBuffer with None, but found a better/safer way.

jstarry · 2024-12-09T03:36:43Z

transaction-view/src/transaction_data.rs

+impl TransactionData for bytes::Bytes {
+    #[inline]
+    fn data(&self) -> &[u8] {
+        self.as_ref()
+    }
+}


This doesn't seem to be used anymore?

yeah was lazy and about leaving it because we will eventually move to Bytes. But, better to do that when that happens.

jstarry · 2024-12-09T03:54:17Z

core/src/banking_stage/transaction_scheduler/transaction_state_container.rs

+            bytes.extend_from_slice(data);
+        }
+
+        // Attempt to insert the transaction, storing the frozen bytes back into bytes buffer.


What do you mean by "storing the frozen bytes back into [the] bytes buffer"? Aren't they already in there?

Forgot to remove the comment when I updated the code to not use Bytes. Originally, this had a state transition in an enum where we need to move the frozen bytes back into bytes_buffer.

It's 100% simpler this way, and I can just delete this comment since it is just entirely wrong.

jstarry · 2024-12-09T04:14:20Z

core/src/banking_stage/transaction_scheduler/transaction_state_container.rs

+        if self.inner.id_to_transaction_state.len() == self.inner.id_to_transaction_state.capacity()
+        {
+            return true;
+        }


My understanding of the code is that it's not actually possible for this condition to be hit, is that right? Every time we insert into the container, we will remove an element if the container is at capacity. So this extra check is just a sanity check? Should we add this sanity check to TransactionStateContainer::insert_new_transaction too?

It's a sanity check to make sure we're not expanding the Slab. I don't think it makes sense to just fail silently like this - I'll create a helper method and assert that this condition is never hit.

jstarry · 2024-12-09T04:19:02Z

core/src/banking_stage/transaction_scheduler/transaction_state_container.rs

+        let bytes_entry = &mut self.bytes_buffer[transaction_id];
+        // Assert the entry is unique, then copy the packet data.
+        {
+            assert_eq!(Arc::strong_count(bytes_entry), 1, "entry must be unique");


Can you add a safety comment here explaining that the only possible clone is stored in the slab container and is wrapped in a RuntimeTransaction<ResolvedTransactionView<...>> type which doesn't implement Clone and doesn't allow the underlying Arc to be referenced?

jstarry · 2024-12-09T04:23:07Z

core/src/banking_stage/transaction_scheduler/transaction_state_container.rs

+pub(crate) struct SuccessfulInsert {
+    pub state: TransactionState<RuntimeTransaction<ResolvedTransactionView<SharedBytes>>>,
+}


nit: doesn't seem like we need this struct. Maybe just a type alias instead?

22c9c9c
and
31da1b2

jstarry · 2024-12-09T04:55:00Z

core/src/banking_stage/transaction_scheduler/prio_graph_scheduler.rs

+                    // currently have ownership of the transaction, and
+                    // therefore may have a reference to the backing-memory
+                    // that the container expects to be free.
+                    drop(transaction);


Is this drop actually necessary before the remove still? I think it should get dropped when it goes out of scope right after the remove and we have a mutable reference to container still so there won't be any inserts into the container mixed in

👍 artifact of the fact I did the clear-ing in remove_by_id which required mutability of the Arc/Bytes.

No longer necessary!

jstarry · 2024-12-09T07:51:41Z

core/src/banking_stage.rs

+            warn!(
+                "Forwarding only supported for `Sdk` transaction struct. Overriding to use `Sdk`."
+            );


Let's skip the warning log if transaction_struct was already set to sdk

apfitzge · 2024-12-10T15:14:27Z

core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs

    fn receive_and_buffer_packets(
        &mut self,
        container: &mut Self::Container,
        timing_metrics: &mut SchedulerTimingMetrics,
        count_metrics: &mut SchedulerCountMetrics,
        decision: &BufferedPacketsDecision,
-    ) -> bool;
+    ) -> Result<usize, ()>;


This change was to improve the testing which was seeing some inconsistent failures, due to timing out in this call.

Those tests now loop until this call returns Ok(0) - or an error.

Natural question - Why did this only pop up for TransactionView?

The receive_and_buffer timeout for view is done differently than for sdk types.
The sdk types have a receive timeout, i.e. a time limit to receive packets, and then will deserialize all packets it received regardless of the time that takes.

The view type has an overall timeout so that it does not take longer than specified time for all operations - receive AND parsing.

Since receiving takes almost no time/work, this consistently would receive all the batches sent in the sdk variant. and then it would take however long to deserialize them - whether or not OS scheduler decides to rug the thread doesn't matter. If the view variant of the test gets rugged during parsing, then it can time out and not receive the next message.

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch 4 times, most recently from 2876a3f to c868ff5 Compare November 27, 2024 17:17

apfitzge commented Dec 2, 2024

View reviewed changes

apfitzge marked this pull request as ready for review December 2, 2024 18:50

apfitzge requested a review from a team as a code owner December 2, 2024 18:50

apfitzge requested review from jstarry and tao-stones December 2, 2024 18:50

apfitzge self-assigned this Dec 2, 2024

apfitzge requested a review from bw-solana December 2, 2024 20:23

apfitzge commented Dec 3, 2024

View reviewed changes

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from f87bd2f to 2d9cc88 Compare December 3, 2024 17:54

pgarg66 reviewed Dec 3, 2024

View reviewed changes

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from 2d9cc88 to e310dd7 Compare December 5, 2024 17:06

apfitzge marked this pull request as draft December 5, 2024 22:06

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from 4e927a9 to a8c7a41 Compare December 6, 2024 14:00

apfitzge marked this pull request as ready for review December 6, 2024 15:29

bw-solana reviewed Dec 9, 2024

View reviewed changes

jstarry reviewed Dec 9, 2024

View reviewed changes

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from 6ccd99a to 19fb288 Compare December 9, 2024 23:13

apfitzge commented Dec 10, 2024

View reviewed changes

ryoqun mentioned this pull request Dec 11, 2024

tracking issue (brain dump): unified scheduler in block-production #3832

Open

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from 2b8807d to d579b84 Compare December 11, 2024 18:32

apfitzge requested a review from jstarry December 11, 2024 20:46

apfitzge added 5 commits December 12, 2024 16:51

impl TransactionData for Bytes

14b535a

Optional packet

b40e9ed

TransactionViewStateContainer

c252e14

TransactionViewReceiveAndBuffer

b1ee578

spawn_scheduler_and_workers

688df33

apfitzge added 21 commits December 12, 2024 16:55

convert benches/tests to be switchable

654f825

add cli hookup for validator

c41984f

test both ReceiveAndBuffer in scheduler_controller

229d5cc

Fix bytes leak on retry

ba31e6a

receive_and_buffer exit condition fix

f136799

lock bank_forks less often

b93190f

remove references

2754269

minor sleeping efficiency improvement

5ddee11

impl TransactionData for Arc<Vec<u8>>

1f52098

SharedBytes = Arc<Vec<u8>>

98d9d94

only warn about forwarding if not already using Sdk

37ea840

fix bug on should_forward

95dded9

test_initialize_should_forward

dffbc97

remove option

e0f2a32

remove impl TransactionData for Bytes

32af3d2

get_vacant_map_entry

8f72d90

Safety comment

00fe6ed

remove SuccessfulInsert type

3c4dd24

type aliases

e077b36

remove explicit drop

3fd20eb

Remove timeout from testing

05779cd

apfitzge force-pushed the new_receive_and_buffer_2024_11_26 branch from d579b84 to 05779cd Compare December 12, 2024 22:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TransactionView: ReceiveAndBuffer #3820

TransactionView: ReceiveAndBuffer #3820

apfitzge commented Nov 27, 2024

apfitzge Dec 2, 2024

apfitzge Dec 3, 2024 •

edited

Loading

pgarg66 left a comment

bw-solana left a comment

bw-solana Dec 9, 2024

apfitzge Dec 11, 2024

bw-solana Dec 9, 2024

apfitzge Dec 9, 2024

bw-solana Dec 11, 2024

jstarry left a comment

jstarry Dec 4, 2024

apfitzge Dec 9, 2024

jstarry Dec 4, 2024

apfitzge Dec 9, 2024

apfitzge Dec 9, 2024

jstarry Dec 9, 2024

apfitzge Dec 9, 2024

apfitzge Dec 9, 2024

jstarry Dec 9, 2024

apfitzge Dec 9, 2024

jstarry Dec 9, 2024

apfitzge Dec 9, 2024

apfitzge Dec 9, 2024

jstarry Dec 9, 2024

apfitzge Dec 9, 2024

jstarry Dec 9, 2024

apfitzge Dec 9, 2024

jstarry Dec 9, 2024

apfitzge Dec 9, 2024

apfitzge Dec 9, 2024

jstarry Dec 9, 2024

apfitzge Dec 9, 2024

apfitzge Dec 10, 2024

TransactionView: ReceiveAndBuffer #3820

Are you sure you want to change the base?

TransactionView: ReceiveAndBuffer #3820

Conversation

apfitzge commented Nov 27, 2024

Problem

Summary of Changes

Choose a reason for hiding this comment

apfitzge Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

pgarg66 left a comment

Choose a reason for hiding this comment

bw-solana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge Dec 3, 2024 •

edited

Loading